use local serialization lib, part-1#30
Conversation
yb01
requested review from
Sindica,
q131172019 and
sonyafenge
and removed request for
Sindica and
sonyafenge
| @@ -0,0 +1,171 @@ | |||
| /* | |||
| Copyright 2015 The Kubernetes Authors. | |||
There was a problem hiding this comment.
I understand the file was copied from Arktos. But the copyright header here looks a bit strange since all others do not have the header.
| Identifier() Identifier | ||
| } | ||
|
|
||
| //// MemoryAllocator is responsible for allocating memory. |
There was a problem hiding this comment.
A lot of code commented out. Can those be simply deleted?
| //type lengthDelimitedFramer struct{} | ||
| // | ||
| //// NewFrameWriter implements stream framing for this serializer | ||
| //func (lengthDelimitedFramer) NewFrameWriter(w io.Writer) io.Writer { |
There was a problem hiding this comment.
Why FrameWriter here was commented?
| wr := apiTypes.WatchRequest{} | ||
|
|
||
| err = json.Unmarshal(body, wr) | ||
| r, err := s.Decode(body, wr) |
There was a problem hiding this comment.
Have you tested protobuf encoding/decoding object as map index?
| @@ -103,18 +114,28 @@ func TestHttpGet(t *testing.T) { | |||
|
|
|||
| dec := json.NewDecoder(recorder.Body) | |||
There was a problem hiding this comment.
Here only test Json as client, right? Any plan for protobuf client test?
|
|
||
| type Installer struct { | ||
| dist di.Interface | ||
| js serializer.Serializer |
There was a problem hiding this comment.
names and types are confusing. Can you make the name more self explainable or change the type?
| content := req.Header.Get("Content-Type") | ||
| if content == "application/json" { | ||
| desiredSerializer = i.js | ||
| } else { |
There was a problem hiding this comment.
Default to protobuf does not seem right
|
|
||
| // Write writes a single frame to the nested writer, prepending it with the length in | ||
| // in bytes of data (as a 4 byte, bigendian uint32). | ||
| func (w *lengthDelimitedFrameWriter) Write(data []byte) (int, error) { |
There was a problem hiding this comment.
When you have time, can you make a quick introduction to the framer? Many thanks!
|
put this PR on hold for now and to focus on the end to end test effort first. |
|
close this PR. new PR #107 has the changes |
2 participants
code-level reuse the serializer package from the apimachinery/serializer package.
the heavy meta data, such as multiple type, versioning conversion in the serializer package has been removed.